chore: Refactoring of CometError/SparkError #655
Conversation
|
@Blizzara @advancedxy fyi |
native/spark-expr/src/error.rs
Outdated
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Self::ArithmeticOverflow { from_type } => | ||
| write!(f, "[ARITHMETIC_OVERFLOW] {from_type} overflow. If necessary set \"spark.sql.ansi.enabled\" to \"false\" to bypass this error."), |
There was a problem hiding this comment.
nit: should we keep the earlier comment, i.e. // Note that this message format is based on Spark 3.4?
| use std::fmt::{Display, Formatter}; | ||
|
|
||
| #[derive(Debug)] | ||
| pub enum SparkError { |
There was a problem hiding this comment.
I think this is not specified to expression but also Spark operator. But since we don't have spark-operator crate, it looks okay to have it in spark-expr now.
native/spark-expr/src/error.rs
Outdated
| impl Display for SparkError { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Self::ArithmeticOverflow { from_type } => |
There was a problem hiding this comment.
could we leverage the error macro in the previous impl?
#[error("[ARITHMETIC_OVERFLOW] {from_type} overflow. If necessary set \"spark.sql.ansi.enabled\" to \"false\" to bypass this error.")]
Which seems slightly readable.
There was a problem hiding this comment.
The only reason I didn't do this was because I was trying to avoid adding a dependency on thiserror, but from looking at their documentation it seems like this isn't really an issue because it is just a derive macro. I will make this change.
Which issue does this PR close?
Part of #630
Rationale for this change
These changes are extracted from the larger WIP PR #654 and are a first step to being able to move the
castexpression to the newspark-exprcrate.What changes are included in this PR?
SparkErrorinto its own source fileArithmeticOverflow,CastInvalidValue,CastOverFlow, andNumericValueOutOfRangefromCometErrortoSparkErrorCometError::Spark(SparkError)variantHow are these changes tested?
Existing tests